-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Design for single-use-token (SUT) authenticator #2971
Conversation
f8256db
to
4cc5ea1
Compare
I don't love the term |
|
||
The following diagram shows the basic workflow for using a SUT to authenticate | ||
to Conjur. The first step is to call the | ||
`GET /authn-sut/<authenticator-name>/<account>/login` endpoint with the user's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having this authenticator avoid the individual service_id
pattern (as we do with authn-gcp
). It doesn't seem like there will be any specific details that will necessitate having multiple "versions" of the single-use token authentication workflows.
The pattern we use in the GCP authenticator is to nest the key bits under the authenticator level:
- !policy
id: conjur
body:
- !policy:
id: authn-sut
body:
- !webservice
- !group
id: authenticatable
annotations:
description: Group with permission to authenticate using this authenticator
- !permit
role: !group authenticatable
privilege: [ read, authenticate ]
resource: !webservice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with that. I'll update the doc to remove the service-id pattern.
Although it likely makes sense to limit the initial scope to Conjur Users, it's important to enable this functionality for Hosts as well. There are a number of interesting use cases that will be enabled with support for SUT for hosts. For awareness, there is a push to remove the API key as an authentication mechanism. When that fully comes to fruition, I suspect that adding an As a final note, the decoupling of identity from authentication will likely require is to support "default" authentication mechanisms. It's quite possible we can support this behavior via Policy Factories, but we may need a more native solution at some point. |
I feel that token is too generic and confusing, since we already have auth tokens. Maybe authn-nonce or something similar? |
Absolutely, I see no reason not to allow this for hosts.
To clarify, are you saying that API key authentication will no longer be enabled by default, and it'll need to be moved to an authn-apikey authenticator that will need to be enabled manually? How will this affect the single use token authenticator?
Again to clarify, do you mean we'll want to have this authenticator enabled by default without requiring the administrator to create a policy for it? |
0b1b7b5
to
b6e132b
Compare
The following diagram shows the workflow for using a SUT to authenticate to | ||
Conjur via the PSM integration. The first step is to call the | ||
`GET /authn-sut/<account>/login` endpoint with the user's | ||
credentials to generate a SUT. The SUT is then passed to the Conjur UI via a URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any security concerns around passing the SUT as a URL parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the most secure way of passing around sensitive data, which is why we're suggesting using a single-use, short-lived token instead of using the 8 minute auth token as was previously considered. This way even if it's leaked the window for exploitation is low. Currently PSM does not support POST requests so this the only way we can do it that will work for them.
is used, a feature not easily implemented with JWTs. This would make it | ||
impossible to prevent replay attacks. | ||
|
||
##### Open Questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a user have more than one SUT active at a time? (I'd say no - a user requesting a SUT should immediately expire any existing SUTs for that user.)
Should the authn-sut throttle requests over a certain rate? (Maybe... need to think through if having a lot of SUTs active at once would be additionally problematic...)
implementation will follow the existing authenticator plugin pattern, as | ||
documented in [AUTHENTICATORS.md](../AUTHENTICATORS.md) | ||
|
||
## Security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about taking a page from OIDC to add security here. What if PSM (or any client) had to generate a unique code and passed the hash of that code to the authn-sut/login endpoint. Then, PSM sends the actual code to Conjur UI. Conjur UI includes that along with the SUT to Conjur. Conjur applies the same hash (we'd have to pick a single algorithm - maybe SHA256) to the code and only honors the SUT if that matches the request code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filling this out more explicitly - I'm modeling this on PKCE in OIDC.
- PSM generates a GUID for a specific single use token request.
- PSM SHA256 hashes the GUID and sends the resulting hash value and the transformation method (currently only "sha256" supported, but this will make it easier for us to introduce post-quantum crypto handling or other algorithmss later) to the endpoint for the SUT.
- The single use endpoint includes the received hash value and transformation methods as additional columns in the database where the token is stored. The token is returned to PSM.
- PSM sends the token and the original (unhashed) GUID to Conjur UI / Conjur. The verification process includes a step that takes the GUID, applies the transformation method associated with the token, and verifies that it gets the same result as was written from the original request. If it does not get the same result, the token is rejected and declared invalid immediately.
Key pieces:
- The GUID itself is never sent anywhere until it's used with the token to prevent interception.
- The GUID ties a single use token to a particular request, so the token can't be used elsewhere.
- The transformation method must be one way, so even if someone got a hold of the database, they couldn't reverse engineer the GUID and gain access to using the token.
- Even though we only support one transformation method at the start, we have the mechanism in place to allow other choices in the future to make upgrades and introducting new functionality easier.
|
||
The following diagram shows the workflow for using a SUT to authenticate to | ||
Conjur on behalf of a 3rd party client. The first step is to call the | ||
`GET /authn-sut/<account>/login` endpoint with the user's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have registered client IDs and require the client to sign this request? We'd have to have a keypair set up to allow the client to request a SUT as part of the configuration, but it would stop the endpoint being used by non-authorized services.
1a86ce2
to
67ca7da
Compare
- Should we provide a way for clients to register a key pair to use for | ||
signing the SUT generation request? This would allow us to verify that the | ||
request came from a trusted client. | ||
- Should we create a job to clean up expired SUTs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. We can use Cron as it's already present in Conjur and Conjur Enterprise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvanderhoof Where is Cron used in Conjur? I'm not seeing it in this repository and if we need to add it that'll require some more story points in the work estimate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake. Cron is part of Conjur Enterprise, not Conjur. Two options that come to mind are:
- Remove expired single-use tokens during creation, exchange, or both.
- Create a service like the variable rotation service to run and periodically remove expired tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK got it. For now I'll update this doc to allow for some extra points for figuring that out during the implementation.
673cdce
to
8233001
Compare
Code Climate has analyzed commit 88d4451 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 88.4% (-0.1% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
authenticator architecture, so we only need to be concerned with the security of | ||
the SUT and the implementation of the SUT authenticator plugin. | ||
|
||
## Testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szh nice list of test scenarios. I had a couple comments/questions..
- what if ldap is enabled for Conjur-UI login by default (same for authn-oidc but this is probably fine as local auth is allowed vs ldap)?
- check that tokens are not logged (even though they are short lived)
- how about
authn-temp
as credential name? - will there be a status endpoint for authn_sut?
- could you define more than one authn_sut authenticator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll clarify these. I think authn-temp is too vague - it makes it sound like the authenticator itself is temporary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure about authn-ldap - I'll need some clarification from someone more familiar with UI project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some minor things and 1 thing (audit log & higher-than-debug logging of one user trying to use another user's token) that I think is important, but not enough to merit another cycle of review. If you feel that merits changes that invalidate this approval, I'll do approve again quickly.
> console. | ||
|
||
The SUT authenticator will be the mechanism used to authenticate the user to | ||
Conjur when they click "Connect" on a Conjur instance in PVWA. PSM will generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: PSM isn't generating the SUT. Conjur generates the SUT and passes it back to PSM.
| 10 | Invalid code challenge algorithm in /login request headers | Code challenge algorithm not supported | | ||
| 11 | Missing code challenge algorithm in /login request headers | Code challenge algorithm not provided | | ||
| 12 | Missing code verifier in /authenticate request body | Code verifier not provided | | ||
| 13 | Code verifier does not match code challenge | Code verifier does not match code challenge | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might make this error message something like "Invalid code verifier". No need to disclose anything that could be construed as the verification method necessarily.
| 1 | After validating SUT | Single use token for {0-role-name} has been validated successfully | | ||
| 2 | Attempt to use expired SUT | Single use token for {0-role-name} has expired | | ||
| 3 | Attempt to use used SUT | Single use token for {0-role-name} has already been used | | ||
| 4 | Attempt to use SUT for different user | Single use token for {0-role-name} is not valid for {1-role-name} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be only a debug message. This might be an indicator of an active attack and thus something we should be surfacing to the audit DB and maybe even at a Warning log level.
| Authenticate with SUT - Expired SUT | Sad | Verify that a user cannot authenticate to Conjur with an expired SUT | | | | ||
| Authenticate with SUT - Invalid code verifier | Sad | Verify that a user cannot authenticate to Conjur with an invalid code verifier | | | | ||
| Authenticate with SUT - Incorrect user | Sad | Verify that a user cannot authenticate to Conjur with a SUT for a different user | | | | ||
| Generate & Authenticate - Multiple SUTs | Sad | Generate two SUTs for a user. Verify that the first one cannot be used after the second is created. | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of Jason's desire to minimize cucumber tests stated above, do we need all these Sad path tests here as well as in unit tests? 1 happy path test (generate the SUT and authenticate) and 1 sad path test to make sure errors make it to where they should be seem like enough given the set of unit tests.
Moved to GitHub Enterprise. See PR #8 in the repo there. |
Desired Outcome
Our first pass at a UI-PSM integration was rejected for security reasons and the more secure option likely requires us to create a new token authenticator for Conjur. Create a solution design for that and figure out how large of an effort it would be.
Implemented Changes
Created a solution design document for a new single-use-token (SUT) authenticator
Connected Issue/Story
CyberArk internal issue ID: CNJR-2514